Skip to content

Implement WAFPolicy controller #3532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: feat/nap-waf
Choose a base branch
from
Open

Conversation

ciarams87
Copy link
Contributor

@ciarams87 ciarams87 commented Jun 21, 2025

Proposed changes

Problem:
As a user of NGF
I want my WafPolicy configuration applied to NGINX for the Gateway or Route scope
So that I can enable WAF protection on my traffic

Solution: Implement the WAFPolicy controller.

Testing: Over 90% unit test coverage, and manual testing in a GKE cluster

Closes: #3454

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

NONE

@ciarams87 ciarams87 force-pushed the feat/wafpolicy-ctlr branch from 469bfd8 to 77a793f Compare June 25, 2025 08:06
@github-actions github-actions bot removed the helm-chart Relates to helm chart label Jun 25, 2025
@ciarams87 ciarams87 force-pushed the feat/wafpolicy-ctlr branch from ddeaa8e to 499cfed Compare June 25, 2025 17:04
@ciarams87 ciarams87 force-pushed the feat/wafpolicy-ctlr branch from 499cfed to 752013c Compare July 3, 2025 12:19
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 3, 2025
@ciarams87 ciarams87 force-pushed the feat/wafpolicy-ctlr branch 3 times, most recently from ad59974 to 047418f Compare July 3, 2025 19:02
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Jul 3, 2025
@ciarams87 ciarams87 marked this pull request as ready for review July 3, 2025 19:03
@ciarams87 ciarams87 marked this pull request as draft July 3, 2025 19:07
@ciarams87 ciarams87 force-pushed the feat/wafpolicy-ctlr branch from 047418f to 28bcbe2 Compare July 3, 2025 21:42
@ciarams87 ciarams87 marked this pull request as ready for review July 3, 2025 22:01
@ciarams87 ciarams87 requested a review from a team July 3, 2025 22:01
Copy link
Collaborator

@sjberman sjberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small things, but overall looks pretty good!

if wp.Spec.PolicySource != nil && wp.Spec.PolicySource.FileLocation != "" {
fileLocation := wp.Spec.PolicySource.FileLocation
bundleName := helpers.ToSafeFileName(fileLocation)
bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First arg is a string literal, doesn't seem like we need to format it.


if secLog.LogProfileBundle != nil && secLog.LogProfileBundle.FileLocation != "" {
bundleName := helpers.ToSafeFileName(secLog.LogProfileBundle.FileLocation)
bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

},
expStrings: []string{
"app_protect_enable on;",
"app_protect_policy_file \"/etc/app_protect/bundles/",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the file name be a part of this?

Comment on lines +70 to +74
func (v Validator) Conflicts(polA, polB policies.Policy) bool {
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polA)
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polB)
return false
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (v Validator) Conflicts(polA, polB policies.Policy) bool {
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polA)
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polB)
return false
}
func (v Validator) Conflicts(_, _ policies.Policy) bool {
return false
}

Comment on lines +496 to +502
if policyKey.GVK != wafPolicyGVK {
continue
}

if !policy.Valid {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could combine these two

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

Implement WafPolicy controller & generate the correct NGINX configuration
3 participants